KAFKA-18600 Cleanup NetworkClient zk related logging#18644
Conversation
| log.debug("Node {} has finalized features epoch: {}, finalized features: {}, supported features: {}, ZK migration ready: {}, API versions: {}.", | ||
| log.debug("Node {} has finalized features epoch: {}, finalized features: {}, supported features: {}, API versions: {}.", | ||
| node, apiVersionsResponse.data().finalizedFeaturesEpoch(), apiVersionsResponse.data().finalizedFeatures(), | ||
| apiVersionsResponse.data().supportedFeatures(), apiVersionsResponse.data().zkMigrationReady(), nodeVersionInfo); |
There was a problem hiding this comment.
(Unrelated to this PR) @mumrah Do we have a plan to remove this field from the protocol api? We would need to ensure it's done in a compatible way, but it would be nice not to have this field in request logs and general logging.
There was a problem hiding this comment.
Since it's a tagged field and 4.0 cannot be the target of a ZK migration, we could just remove it without a version bump. However, we might consider keeping it around so that 4.0+ controllers can reject errant registrations from a Zk broker (like due to a misconfiguration).
There was a problem hiding this comment.
Interesting, would this be from a broker that is being migrated?
There was a problem hiding this comment.
Oh actually I was confusing this with the same field in the broker registration request. Looking at usages in 3.9, I'm not sure we ever used this field. I'll dig in a bit on this.
There was a problem hiding this comment.
This field (ZkMigrationReady in ApiVersionsResponse) was last used in 3.6. Starting in 3.7, we used the controller registration RPC in lieu of this field. So, it seems a bit like dead code at this point. We should probably leave the field in the RPC so we don't reuse the tagged field number, but we can set the max versions to v4 and remove the usages in the non-generated code.
There was a problem hiding this comment.
we can set the max versions to v4
Pardon me, it seems to me the max version is already set to v4 (ApiKeys.API_VERSIONS.latestVersion()). Please correct me if I misunderstand anything.
we might consider keeping it around so that 4.0+ controllers can reject errant registrations from a Zk broker (like due to a misconfiguration).
it seems that is addressed already.
remove the usages in the non-generated code.
I prefer to remove isMigratingZkBroker and IsMigratingZkBroker from non-generated code. similar to my previous comment #17293 (comment)
|
Hello @mumrah, If there are no further changes needed for this PR, perhaps it can be merged. Thanks. |
This PR removes associated logging within NetworkClient to reduce noise and streamline the client code. Reviewers: Ismael Juma <ismael@juma.me.uk>, David Arthur <mumrah@gmail.com>, Chia-Ping Tsai <chia7712@gmail.com>
4 participants
This PR removes associated logging within NetworkClient to reduce noise
and streamline the client code.
Reviewers: Ismael Juma ismael@juma.me.uk, David Arthur
mumrah@gmail.com, Chia-Ping Tsai chia7712@gmail.com